Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edits to improve functionality and documentation #889

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Luisg55
Copy link

@Luisg55 Luisg55 commented Apr 21, 2017

• I tried to improve documentation by contributing to the initial README and the tutorial README

• I tried improving functionality of the fispact.py, specifically the isFisII() function

added check for file being FISPACT instead of FISPACT-II for program user's convenience
added a line that encouraged users to be appropriate on the forum
@@ -224,3 +224,5 @@ it is as easy as forking the repository on GitHub, making your changes, and
issuing a pull request. If you have any questions about this process don't
hesitate to ask the mailing list (https://groups.google.com/forum/#!forum/pyne-dev,
pyne-dev@googlegroups.com).

Make sure to be appropriate on the forum and provide as clear of an explanation (with examples if possible) of any problems or questions you might have.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a code of conduct for pyne, but until then, "appropriate" is vague. So, perhaps the appropriate wording here is just "Please provide as clear of an explanation (with examples if possible) of any problems or questions you might have."

@@ -23,3 +23,4 @@ and IPython (v1.0+). Note that only `yt-3.0 <https://bitbucket.org/yt_analysis/y
supports PyNE. Be sure to use this version of yt rather
than the more stable v2.x series.

"""I suggest adding a reference for people to contact incase they have questions here: _________"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than have then contact people individually, I'd actually suggest that they submit a ticket in the issues system or send an email to pyne-dev. Perhaps the text you have above would be appropriate here:

If you have any questions about this process, don't hesitate to ask the mailing list (https://groups.google.com/forum/#!forum/pyne-dev, pyne-dev@googlegroups.com). pyne-dev@googlegroups.com). Please provide as clear of an explanation (with examples if possible) of any problems or questions you might have.

if v == "FISPACT-II":
"""boolean check if file is fispact-ii output (or fispact output for the program user's convinience)"""
v = check_fisp_version(data)
if v == "FISPACT":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test is needed to check this case. Can you add a test that demonstrates the FISPACT (rather than FISPACT-II) behavior here?

"""boolean check if file is fispact-ii output (or fispact output for the program user's convinience)"""
v = check_fisp_version(data)
if v == "FISPACT":
print("File is FISPACT instead of FISPACT-II")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than print, an exception or a warning is more appropriate here. Either raise an exception or execute a warning message with "warn" and return something (true or false) in this if v=FISPACT block.

@katyhuff
Copy link
Member

katyhuff commented May 8, 2017

Hi @Luisg55 . Thanks for your contribution! I made a few suggestions for changes that will be necessary before I can approve his request.

@gonuke gonuke added this to the Deferred milestone Aug 16, 2020
"""boolean check if file is fispact-ii output """
v = check_fisp_version(data)
if v == "FISPACT-II":
"""boolean check if file is fispact-ii output (or fispact output for the program user's convinience)"""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convinience → convenience. As you change this comment, you can fix "fispact-ii" into "FISPACT-II".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants